Skip to content

fix: reset tool aggregator whenever something other than a tool card gets inserted - #870

Merged
will-lamerton merged 9 commits into
Nano-Collective:mainfrom
kishore280:fix/856-tool-aggregator-phase-reset
Aug 21, 2026
Merged

fix: reset tool aggregator whenever something other than a tool card gets inserted#870
will-lamerton merged 9 commits into
Nano-Collective:mainfrom
kishore280:fix/856-tool-aggregator-phase-reset

Conversation

@kishore280

@kishore280 kishore280 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #856.

currentAggregator only reset at the end of a turn, not when a thought or text chunk interrupted a tool-call phase. Tool → Thought → Tool merged the second tool into the first card instead of starting a fresh one.

Review found the same gap in two more spots: a mutating-tool edit card and a plan update also skip the reset, so Tool → Edit → Tool hits the same bug. Pulled the check into one closeAggregatorIfIdle() helper instead of four copies of it.

close() was also re-expanding manually collapsed cards - toggle(force) never used the force argument, so close()'s toggle(false) just flipped whatever state it was already in. Fixed to match ThoughtAggregator's toggle, which already handles force correctly.

Testing the footer-per-turn change from the earlier review turned up two more bugs, both with repro tests: an old turn's copy button could grab a newer turn's text, and interrupting a still-pending tool could duplicate its card with a stuck spinner.

@akramcodez

Copy link
Copy Markdown
Collaborator

Thanks @kishore280 for opening the PR! Please add a changeset to your PR as well.

@akramcodez akramcodez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kishore280, thanks for the fix! I tested it and the tool-call phase separation looks good. I noticed one small regression though: the copy button and timestamp footer can now appear multiple times within the same turn after each text chunk. Please keep a single footer per turn and move it to the latest text block as the response streams. Once that's fixed, this should be good to go.

image

@kishore280
kishore280 force-pushed the fix/856-tool-aggregator-phase-reset branch from d4c0b17 to e4c3bb4 Compare August 13, 2026 12:48
@kishore280

Copy link
Copy Markdown
Contributor Author

Pushed 3 commits. First one is the footer-per-turn change from the review.

The other two are bugs I found while testing it, not extra scope:

  • copy button on an old turn's footer was grabbing the newest turn's text instead of its own — found while testing the footer reuse
  • interrupting a tool call before it finished (thought/text arriving while a tool was still pending) could duplicate its card with a stuck spinner — pre-existing regression from the original [Feature] Preserve tool-call boundaries between thought blocks #856 fix, caught with a repro test

jsdom repros:

# bug 2 — cross-turn footer copy
clicking footer A returned "Response B" -> now correctly returns "Response A"

# bug 3 — duplicate tool card on interrupt
interrupting a pending tool created 2 cards + 2 aggregators -> now 1 of each

…ator-phase-reset

# Conflicts:
#	assets/nanocoder-vscode.vsix
#	plugins/vscode/media/chat-panel.js
@will-lamerton

Copy link
Copy Markdown
Member

Hey @kishore280 - nice catch on the root cause, and mirroring the currentThoughtBox reset is the right shape. Two things before merge:

1. The reset misses the edit-card and plan-card paths. Mutating tools (write_file, replace_file_content, etc.) append straight to messagesContainer via createEditCard and never touch currentAggregator, same for handlePlanUpdate. Since ACP runs a round's tools sequentially, read_file → write_file → read_file puts the second read back into the aggregator above the edit card, which is the same out-of-order symptom #856 describes. Suggest pulling the reset into a helper and calling it from all four insertion points:

function closeAggregatorIfIdle() {
	if (currentAggregator && !aggregatorHasPendingTools(currentAggregator)) {
		currentAggregator.close();
		currentAggregator = null;
	}
}

2. close() re-expands a manually collapsed card. close() calls this.toggle(false) but toggle() takes no argument and just flips isOpen. Pre-existing, but this PR moves close() from once-per-turn to once-per-transition, so it's much easier to hit now. close() { if (this.isOpen) this.toggle(); } covers it.

Minor: the footer rework in 653885cd/e0b41b9a looks correct but isn't mentioned in the title, body, or changeset. Worth a line in the changeset since it changes copy-button behavior, plus a Closes #856 reference to match the other changesets.

@kishore280 kishore280 changed the title fix: reset tool aggregator when a thought or message interrupts a tool-call phase fix: reset tool aggregator whenever something other than a tool card gets inserted Aug 13, 2026
kishore280 and others added 3 commits August 13, 2026 23:15
…thought/message

Also fixes close() re-expanding a manually collapsed card.
Conflicts in plugins/vscode/media/chat-panel.js, both additive:

- 'clear' handler: keep this branch's currentTurnFooter reset alongside
  main's toolKinds.clear().
- ToolAggregator.addOrUpdateTool status block: main split queued
  ('pending' -> circle icon) out of the running case. Queued tools are
  unfinished too, so they mark item.dataset.pending = 'true' and keep the
  aggregator open, same as running ones.
@will-lamerton
will-lamerton merged commit 0cd4158 into Nano-Collective:main Aug 21, 2026
1 check passed
will-lamerton added a commit that referenced this pull request Aug 21, 2026
#870 landed four behaviour fixes with no tests. Each of the 19 added here
was checked against the pre-fix code and fails there:

- tool phases: a thought, reply, edit card or plan card between two tool
  calls starts a fresh aggregated card, and the run stays in one card
  when nothing interrupts it
- an unfinished tool holds its card open, so a late completion reuses the
  original row instead of building a second, permanently spinning one.
  Covers queued ('pending') as well as running, which the merge with the
  queued/running icon split in #847 had to reconcile
- a card the user collapsed by hand is not re-expanded when the phase
  closes, and can still be reopened
- one footer per response rather than one per text segment, following the
  newest text block, copying its own response (every segment of it) and
  not a newer turn's, and dropped when the session is cleared

Harness: element.addEventListener now records handlers and element.click()
dispatches them, so a test can drive the real copy button rather than only
the onclick properties the panel assigns directly; navigator.clipboard
records what was written. createMessageFooter gains a 'message-footer'
class so the footers can be selected the way .tool-card already is.
@will-lamerton

Copy link
Copy Markdown
Member

Thanks for this PR @kishore280 - feel free to add yourself as a contributor to our website via a PR which I will approve :)

https://nanocollective.org/contributors
https://github.com/Nano-Collective/organisation

AryanNandanwar added a commit to AryanNandanwar/nanocoder that referenced this pull request Aug 21, 2026
* Update status badges [skip ci]

* Update status badges [skip ci]

* feat: vscode extension settings tab (Nano-Collective#838)

* feat: implement settings manager and integrate persistent configuration for chat webview

* style: restore settings UI CSS classes lost during merge

* refactor: redesign chat panel UI components and settings layout

* feat: add settings tab to VS Code extension webview for configuration management

* fix: address PR feedback on settings dashboard

* chore: remove accidental tasks.json

* fix(deps): sync pnpm-lock.yaml with package.json

PR Nano-Collective#838 was branched before the chalk and @types/node dependabot bumps
and merged without a rebase, reverting the lockfile to its pre-bump
state while package.json kept the new specifiers. This broke the
release workflow with ERR_PNPM_OUTDATED_LOCKFILE.

Regenerated the lockfile so chalk resolves to 6.0.0 and @types/node to
26.2.0. Verified with pnpm install --frozen-lockfile and tsc --noEmit.

* feat: headless api mode (Nano-Collective#851)

* headless api with --acp flag

* changeset added

* fixed

* removed unused import

* changes made

* unused deps removed

* pass tests

* feat(settings): sunset /setup-providers and /setup-mcp in favour of /… (Nano-Collective#848)

* feat(settings): sunset /setup-providers and /setup-mcp in favour of /settings

Make /settings the single entry point for provider and MCP configuration.

- Remove both commands from the slash registry, delete their stub files,
  and drop the SETUP_PROVIDERS/SETUP_MCP special commands, the mcpWizard
  app mode and its handlers. configWizard stays: the first-run and
  all-providers-failed bootstrap in useAppInitialization still uses it.
- Forward the retired names to the matching /settings tab with a notice
  instead of erroring, since they are still printed across the docs.
- Accept a tab argument: /settings providers, /settings mcp, etc. An
  unknown tab opens the default tab rather than failing.
- Give MCP its own settings tab so tab arguments map onto the tab bar.
- Apply provider edits made from settings to the running session; the
  panel previously discarded the wizard's config path, so a provider
  added there stayed inert until the next launch.
- Update the docs and the /mcp hint that pointed at /setup-providers.

* chore: add changeset for /settings consolidation

* fix(settings): address PR Nano-Collective#848 review feedback

Replays the review fixes on top of the restored sunset commits.

- reloadProviders reloads config and rebuilds the client for the current
  provider/model, leaving messages and the settings panel untouched,
  instead of routing provider edits through handleConfigWizardComplete.
- The providers panel lets the parent own closing, so it no longer sets
  state on an unmounting component.
- The active settings tab is preserved across Tune/IDE launches.
- /settings validates its tab argument through an isSettingsTabId() type
  guard rather than a double cast, and reports unknown tabs instead of
  silently falling back to the default tab.
- SettingsTabId and SETTINGS_TAB_IDS move to settings-constants.ts, so
  app-util no longer pulls the settings panel graph in, and every
  reference uses a top-level `import type`.
- TABS is derived from SETTINGS_TAB_IDS through a Record<SettingsTabId,
  string> label map, so an id without a label is a compile error. This
  caught the missing 'mcp' tab id that /setup-mcp forwards to.
- Drops the removed onEnterConfigWizardMode from context-max-handler.spec.
- Covers the /settings tab argument paths and reloadProviders in specs.

* feat(settings): edit or delete a provider/server from its own row

Completes the remaining settings work from Nano-Collective#832. Selecting a row in the
providers or MCP panel opened the same generic wizard as every other row,
so there was no way to act on one specific entry and no delete affordance.

The wizard steps already had per-entry edit-or-delete modes; they just
could not be reached directly. Adds an optional `initialEditName` threaded
from panel to wizard to step, which opens that entry's edit/delete choice
instead of the template menu. Each row now carries its own entry and a
separate row adds a new one.

Keyed by name rather than index: the panel lists the resolved config
(project + global + env merged) while the wizard loads a single config
file, so positions need not line up and an index would edit the wrong
entry. An unknown name falls back to the normal menu.

BaseConfigWizard is untouched and the prop is optional, so the first-run
onboarding flow is unchanged.

Also points the OrcaRouter provider doc at `/settings providers`; it was
added after the sunset commit was written, so it still referenced the
retired command.

* Update status badges [skip ci]

* add Explain Code / Generate Tests code lenses (Nano-Collective#866)

* feat(vscode): add Explain Code / Generate Tests code lenses

Reaching the agent about a specific function meant switching to the sidebar
and pasting the code in. Every function, method and class now carries two
inline links instead.

Clicking one reveals the chat view and submits the symbol as a prompt: the
instruction, a `file:startLine-endLine` locator and the source fenced with
the document's language. The snippet is inlined rather than attached as an
`@[file]` chip so the agent sees the one symbol that was clicked instead of
the whole file.

Symbols come from `vscode.executeDocumentSymbolProvider`, so the lenses
follow whatever language servers the user already has and nothing here
parses source. Lenses anchor on the symbol's selectionRange - the
declaration line - while the command receives the full body range, so a
preceding doc comment doesn't push the links away from the signature.

A lens can be clicked before the sidebar has ever been opened, so the prompt
is held in `_pendingPrompt` and flushed from `_initializeSessionIfReady`,
which already runs both on webview ready and on ACP connect - whichever
lands last.

The two commands are hidden from the palette: they take a uri and a symbol
range, so a bare invocation would have nothing to act on. `nanocoder.codeLens`
turns the lenses off.

Closes Nano-Collective#750.

* fix(vscode): stop losing an editor prompt across a chat view re-reveal

_isWebviewReady was latched on the first shell and never cleared, so once
the Nanocoder view had been disposed - hidden from its container, or moved
to another one - the next lens click posted runPrompt into a replacement
webview that had not run its script yet. The message went nowhere and the
prompt was cleared, so the click did nothing at all. Reset the flag on
every resolve and drop the view on dispose.

The queued prompt is also bounded now. It used to sit indefinitely when the
CLI was down and then fire from onConnectionReady whenever the agent
happened to come up, answering about code the user had long moved past,
with no feedback in the meantime. It expires after 30s with a warning
instead, and the timer is disarmed once the prompt is handed over.

* fix(vscode): send a lens prompt without disturbing the composer

runPrompt drove the composer: it overwrote chat-input and called
submitMessage, which then folded in attachedPaths and pendingImages. So
clicking Explain Code discarded whatever the user was typing and sent any
file chip or pasted image they had staged for a different question, then
cleared them.

Split the send tail out of submitMessage as dispatchPrompt and route the
editor prompt straight through it, leaving the draft and the staged
context untouched.

* fix(vscode): harden the code lens commands and provider

explainCode/generateTests are hidden from the palette but a keybinding or
another extension can still invoke them bare. uri and range were assumed
present, so openTextDocument(undefined) opened an untitled document and
range.start then threw; guard and point the user at the lenses instead.

Also dispose the onDidChangeCodeLenses emitter with the extension rather
than leaking it, and resolve nanocoder.codeLens against the document so a
folder-level override wins in a multi-root workspace - it is declared
scope: resource to match.

* fix(vscode): address code lens review feedback

Ends the turn when a prompt is rejected for a pending permission: the
webview had already drawn the user bubble and flipped to the loading
state, so with nothing posting prompt_response the composer spun until
the user hit Escape. Pre-existing, but a lens click while an approval
sat unattended made it easy to hit.

Moves NanocoderCodeLensProvider and sendCodeLensPrompt out of
extension.ts into code-lens-provider.ts so they can be tested. The
sibling specs cited as precedent never actually ran - the root AVA glob
only matched source/, and `vscode` is not resolvable outside the
extension host - so this also adds a runtime stub behind a test-only
tsconfig paths entry and widens the glob to plugins/*/src. That revives
acp-client.spec.ts and acp-process-manager.spec.ts as a side effect. The
stub is kept out of the packaged .vsix and the bundle still builds with
--external:vscode.

Caps the source inlined into a lens prompt at 200 lines / 8000 chars,
whichever binds first, with a truncation marker. Generate Tests on a
large class would otherwise paste the whole body into the conversation.
The file:start-end locator survives truncation, so the agent can still
read the rest.

ChatWebviewProvider now implements Disposable and is registered with the
extension's subscriptions, so the pending-prompt timer cannot outlive
it. The clear deliberately does not happen in onDidDispose: a view
disposal is usually a re-reveal in progress, and dropping the queued
prompt there would reintroduce the bug 9a9d5bf fixed. onDidDispose also
guards on view identity so a late teardown cannot null out a newer view.

Adds Constructor to LENS_SYMBOL_KINDS - every sibling method already got
a lens.

* fix(test): boot mention-utils.js in the chat panel harness

chat-panel.html loads mention-utils.js ahead of chat-panel.js, and
chat-panel.js destructures globalThis.NanocoderMentionUtils at IIFE time.
The harness only ran chat-panel.js, so every spec that booted a panel died
on `Cannot destructure property 'findMentionQuery' of
'globalThis.NanocoderMentionUtils' as it is undefined` before reaching its
assertions — 28 failures across chat-panel-thoughts and
chat-panel-tool-cards.

The harness (Nano-Collective#847/Nano-Collective#867) and the mention-utils extraction (Nano-Collective#842) landed
independently, so neither PR saw the break; it only appears once both are
on main. Loading the two scripts into the VM in the same order the page
does fixes it.

* fix(vscode): drop duplicate MENTION_UTILS_SOURCE in chat-panel harness

Nano-Collective#866 landed the constant twice at module top level, which is a
redeclaration error. main has been failing test:types, test:lint,
test:format, the build and every spec that imports the harness since.

The surviving declaration is the mediaUrl() one-liner, matching
PANEL_SOURCE next to it; the deleted block's comment is already covered
by the file docblock.

* fix(vscode): silence false-positive semgrep findings in settings-manager

Nano-Collective#838 merged with a red Semgrep check. Both rules are false positives:
resolveConfigPath's fileName is a string literal at both call sites, and
updatePreferencesNested's keys come from this file's own call sites, so
neither takes user input. Marked with nosemgrep following the existing
convention in source/config/index.ts, which does the same path.join.

* fix: make the stop button end the turn it was pressed during (Nano-Collective#869)

Closes Nano-Collective#864.

- `acp-session.ts` / `acp-agent.ts`: `cancel()` now only aborts. The controller is rotated by a new `beginTurn()` called synchronously at the top of `prompt()`, before it awaits `acpContentToUserMessage`, so a cancel landing during the file-reading window is no longer dropped.
- `acp-client.ts`: `_clearPendingPermissions()` now runs above the connection guard in `cancel()`, and on `setConnection()` so a crashed-and-restarted agent process cannot leave the client permanently blocked.
- `chat-panel.js`: the ToolAggregator status predicate now treats a `failed` update carrying 'Denied by user' as denied rather than an error.

* test(vscode): run the acp-client and acp-process-manager specs in CI (Nano-Collective#910)

Nano-Collective#842 added plugins/** to ava.files but blocklisted these two by name,
because they transitively import 'vscode', which does not resolve outside
the extension host. That blocker is gone: the root tsconfig now maps the
bare 'vscode' specifier to plugins/vscode/test-stubs/vscode.ts, and tsx
honours those paths - which is exactly what the stub's docstring says it
is for. The exclusions outlived their reason, so six tests silently never
ran, including the two Nano-Collective#869 just added.

Both files pass inside the full serial suite, not just standalone.

Also typecheck the extension. The root tsconfig only includes source/**,
and plugins/vscode builds through esbuild, which strips types without
checking them, so nothing in CI ever typechecked the extension's own
sources or specs. tsc -p plugins/vscode/tsconfig.json is clean today;
this keeps it that way.

* Update status badges [skip ci]

* fix: reset tool aggregator whenever something other than a tool card gets inserted (Nano-Collective#870)

Fixes Nano-Collective#856.

currentAggregator only reset at the end of a turn, not when a thought or text chunk interrupted a tool-call phase, so Tool -> Thought -> Tool merged the second tool into the first card. The reset is now one closeAggregatorIfIdle() helper called from all four insertion points (agent text, thought, plan card, edit card).

Also fixes close() re-expanding a manually collapsed card (toggle now honours its force argument), one footer per agent turn instead of one per text segment, and a turn's copy button grabbing a newer turn's text.

* test(vscode): cover the tool-card and turn-footer fixes from Nano-Collective#870

Nano-Collective#870 landed four behaviour fixes with no tests. Each of the 19 added here
was checked against the pre-fix code and fails there:

- tool phases: a thought, reply, edit card or plan card between two tool
  calls starts a fresh aggregated card, and the run stays in one card
  when nothing interrupts it
- an unfinished tool holds its card open, so a late completion reuses the
  original row instead of building a second, permanently spinning one.
  Covers queued ('pending') as well as running, which the merge with the
  queued/running icon split in Nano-Collective#847 had to reconcile
- a card the user collapsed by hand is not re-expanded when the phase
  closes, and can still be reopened
- one footer per response rather than one per text segment, following the
  newest text block, copying its own response (every segment of it) and
  not a newer turn's, and dropped when the session is cleared

Harness: element.addEventListener now records handlers and element.click()
dispatches them, so a test can drive the real copy button rather than only
the onclick properties the panel assigns directly; navigator.clipboard
records what was written. createMessageFooter gains a 'message-footer'
class so the footers can be selected the way .tool-card already is.

* fix: show resolved path in setup wizard's config location picker (Nano-Collective#855)

* fix: show resolved path in setup wizard's config location picker

* chore: add changeset

* fix: anchor homeRelative to a path separator boundary

* fix: derive path truncation budget from actual terminal width, render path on its own line

* fix: size path truncation budget to the wizard box, not raw terminal width

Also drops the 76-char cap, replaces the null-separator label hack with a
typed path field on LocationOption, adds a regression test that renders
inside the real wizard box, and fixes a circular test assertion.

* fix: indent the path row properly and name its magic numbers

---------

Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Sk Akram <skcodewizard786@gmail.com>
Co-authored-by: Will Lamerton <william.lamerton@gmail.com>
Co-authored-by: Aditya Mishra <adityadevansh2002@gmail.com>
Co-authored-by: Will Lamerton <89926355+will-lamerton@users.noreply.github.com>
Co-authored-by: kishore280 <70363583+kishore280@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Preserve tool-call boundaries between thought blocks

3 participants